Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: pressing enter doesn't set max #1168

Merged
merged 3 commits into from
Nov 16, 2022
Merged

fix: pressing enter doesn't set max #1168

merged 3 commits into from
Nov 16, 2022

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Nov 16, 2022

What it solves

Resolves #1164

How this PR fixes it

The InputValueHelper required the type="button" attribute to prevent it from submitting the parent form and shouldValidate to be true in onMaxAmountClick. I also wrapped InputValueHelper in the correct InputAdornment component.

How to test it

Attempt to send tokens and, after selecting one and entering an amount, press the enter key whilst the field is in focus. Observe no change in value or form submission. Doing so in an empty field should also result in the same.

Clicking "Max" should set the max value, as well as validate the amount, e.g. when no token amount exists, it should show an error regarding a value of 0.

@iamacook iamacook requested a review from schmanu November 16, 2022 11:39
@github-actions
Copy link

github-actions bot commented Nov 16, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

const handleClick = (e: SyntheticEvent) => {
e.preventDefault()
onClick()
onMouseDown()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this still work on mobile devices?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted to using onClick.

@usame-algan
Copy link
Member

The examples in the MUI Documentation don't seem to react to an enter press even though there is an onClick handler attached to it: https://mui.com/material-ui/react-text-field/#input-adornments

It might have to do with the missing InputAdornment wrapper.

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem here isn't onClick per se, but the fact that it thinks the button is a submit button.
type=button alone should already fix it.
You can also add e.preventDefault() in the onClick handler.

Copy link
Member

@schmanu schmanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the previous reviewers:
I think on enter browsers will usually submit the first "submit" button they find in a form.
The standard value of the <button/> type attribute is submit changing it to button should already do the trick.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-type

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 16, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 092f3b0
Status: ✅  Deploy successful!
Preview URL: https://5e91b11a.web-core.pages.dev
Branch Preview URL: https://max-amount.web-core.pages.dev

View logs

@iamacook
Copy link
Member Author

After reverting to onClick, type did the job. I've wrapped it in an InputAdornment nonetheless as it's the "correct" implementation. (e.preventDefault() was already there. I'll leave it as such.)

Copy link
Member

@schmanu schmanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
I tested it again and it works nicely:
Enter submits the form and leads to the next step if the form data is valid.
The next step does not submit on enter (which is correct as there is no text input).

This matches the HTML spec:

When there is only one single-line text input field in a form, the user agent should accept Enter in that field as a request to submit the form.

@katspaugh
Copy link
Member

Note to the QA: let's start testing forms using keyboard shortcuts and not just with a mouse. Tab + Tab + Tab + Enter

@iamacook iamacook merged commit 485869c into dev Nov 16, 2022
@iamacook iamacook deleted the max-amount branch November 16, 2022 16:30
@katspaugh katspaugh mentioned this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Transfer funds] Pressing enter will set the amount to max
4 participants